New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deoptimize ObjectExpression when a __proto__ property is present #4019
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4019 +/- ##
=======================================
Coverage 97.47% 97.47%
=======================================
Files 192 192
Lines 6771 6773 +2
Branches 1983 1984 +1
=======================================
+ Hits 6600 6602 +2
Misses 84 84
Partials 87 87
Continue to review full report at Codecov.
|
src/ast/nodes/ObjectExpression.ts
Outdated
this.getPropertyMap(); | ||
if ( | ||
!this.hasUnknownDeoptimizedProperty && | ||
this.properties.some(prop => { | ||
if (prop instanceof SpreadElement || prop.computed) return false; | ||
if (prop.key instanceof Identifier) return prop.key.name == "__proto__"; | ||
return String((prop.key as Literal).value) == "__proto__"; | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could actually be simplified by using the property map:
this.getPropertyMap(); | |
if ( | |
!this.hasUnknownDeoptimizedProperty && | |
this.properties.some(prop => { | |
if (prop instanceof SpreadElement || prop.computed) return false; | |
if (prop.key instanceof Identifier) return prop.key.name == "__proto__"; | |
return String((prop.key as Literal).value) == "__proto__"; | |
}) | |
const propertyMap = this.getPropertyMap(); | |
if ( | |
!this.hasUnknownDeoptimizedProperty && | |
propertyMap.__proto__?.exactMatchRead |
that means we check if there is at least one property in the object that can be explicitly determined to be __proto__
(and is not a setter). That would also automatically cover cases like this:
var foo = {['__proto__']: obj}
const getProto () => '__proto__' ;
var foo = {[getProto()]: obj}
One thing that is not covered, though is an unknown computed property that resolves to __proto__
. If we would want to include that as well, then it gets kind of complicated.
Another alternative here would be to integrate this into getPropertyMap
. Because in the end, __proto__
does not deoptimize everything, it just changes the prototype so that there may be additional "hidden" properties. So it would be equivalent to the very first property being an "unknown computed" property: All properties below are still valid.
Simplifying it further, one could just treat __proto__
like an unknown computed property when building the property map. And no explicit property deoptimization would need to take place. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Computed properties don't work for this feature (they create a regular property named __proto__
, don't set the prototype), so that's not an issue.
This could definitely be smarter, this patch mostly tries to crudely patch the soundness hole. I could switch it over to using an unknown computed property, if you think that works. I'll look into it next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: __proto__
needs to be a written string in object expressions. Ouch. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a patch that moves this to getPropertyMap
as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I took the liberty to fix the formatting as apparently the commit hooks is not running for you (maybe a problem with the setup?). In any case, I guess we should move to ESLint with an included prettier config eventually...
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install marijnh/rollup#deoptimize-proto Once a build has completed, you can load it into the REPL by inserting the CircleCI build number of the ci/circleci: node-v12-latest build into the link below (unfortunately, we cannot auto-update this comment for PRs from forks at this time):
|
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: #4014
Description
Works around issues with effect tracking in object literals with a prototype (specified with a
__proto__
or"__proto__"
property) by completely deoptimizing nodes with such a property.